Fix: Improve Anthropic / Claude API support#62
Conversation
- Add tool call collection in non-streaming Anthropic messages - Implement system prompt normalization ([System]: prefix) - Fix token counting to use resolved model instance - Add Claude model name fuzzy matching (date suffix stripping, separator normalization) - Implement /v1/messages/count_tokens endpoint for token pre-counting - Add GET / and HEAD / root endpoint handlers for health probes - Add image placeholder handling for tool_result blocks
|
new update: improved performance of dashboard |
There was a problem hiding this comment.
Pull request overview
This PR improves Claude/Anthropic compatibility in the Copilot API gateway while adding instrumentation and repeatable performance tooling to validate dashboard/runtime behavior.
Changes:
- Extend the Anthropic Messages implementation (tool-use blocks,
/v1/messages/count_tokens, model fuzzy-matching, image placeholders, improved token counting). - Reduce dashboard re-render churn by shifting stable updates to message-based snapshots and adding audit “snapshot” fetching.
- Add perf instrumentation (counters/phases + CPU profiling) and scripts to benchmark/profile the extension host.
Reviewed changes
Copilot reviewed 10 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/PerfMetrics.ts | Adds static perf counters + phase timing snapshots for webview/render activity. |
| src/services/ExtensionHostProfiler.ts | Adds Node inspector CPU profiling for extension-host perf runs. |
| src/extension.ts | Adds perf commands and makes gateway initialization single-flight via gatewayPromise. |
| src/CopilotPanel.ts | Moves stable UI updates to message snapshots, adds audit snapshot flow, and records perf metrics. |
| src/CopilotApiGateway.ts | Enhances Anthropic support (tool_use blocks, token count endpoint, caching, model fuzzy match) and audit IP tracking. |
| perf/run-runtime-bench.cjs | Adds a runtime micro-benchmark harness with VS Code mocks. |
| perf/run-dashboard-profile.cjs | Adds an extension-host driven dashboard open CPU profile runner. |
| perf/extension-host/index.cjs | Adds extension-host test entry that drives perf commands and writes reports. |
| perf/README.md | Documents perf scripts and outputs. |
| package.json | Adds perf scripts and updates extension version. |
| dist/extension.js | Updates the bundled output to reflect source changes. |
| .gitignore | Ignores perf artifacts, VS Code test dirs, vsix/cpuprofile outputs, etc. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (gatewayPromise) { | ||
| return gatewayPromise; | ||
| } | ||
|
|
||
| // Hook up listeners | ||
| context.subscriptions.push(gw.onDidChangeStatus(async () => { | ||
| await updateStatusBar(); | ||
|
|
||
| // Notifications | ||
| const status = await gw.getStatus(); | ||
| if (status.running && !wasRunning) { | ||
| const config = vscode.workspace.getConfiguration('githubCopilotApi.server'); | ||
| if (config.get<boolean>('showNotifications', true)) { | ||
| // Show actual LAN IP instead of 0.0.0.0 when bound to all interfaces | ||
| const displayHost = (status.config.host === '0.0.0.0' && status.networkInfo?.localIPs?.length) | ||
| ? status.networkInfo.localIPs[0] | ||
| : status.config.host; | ||
| const protocol = status.isHttps ? 'https' : 'http'; | ||
| const selection = await vscode.window.showInformationMessage( | ||
| `GitHub Copilot API Server started at ${protocol}://${displayHost}:${status.config.port}`, | ||
| 'Open Dashboard' | ||
| ); | ||
| if (selection === 'Open Dashboard') { | ||
| void vscode.commands.executeCommand('github-copilot-api-vscode.openDashboard'); | ||
| gatewayPromise = (async () => { | ||
| const gw = new CopilotApiGateway(output, statusItem, context); | ||
| gateway = gw; | ||
| context.subscriptions.push(gw); | ||
|
|
There was a problem hiding this comment.
getGateway() caches the in-flight initialization in gatewayPromise, but if the async initializer throws/rejects, gatewayPromise will stay set to a rejected promise and all future callers will permanently fail (no retry). Consider wrapping the initializer so on failure you clear gatewayPromise (and gateway if it was set) before rethrowing.
| // Get client IP for rate limiting | ||
| const clientIp = this.getClientIp(req); | ||
| this.requestIpMap.set(requestId, clientIp); | ||
|
|
||
| // Root endpoint — responds to both GET and HEAD (used by clients as a connectivity probe) | ||
| if (url.pathname === '/') { | ||
| const body = JSON.stringify({ |
There was a problem hiding this comment.
The root / handler returns early without calling logRequest(), but requestIpMap.set(requestId, clientIp) has already happened. That leaves an entry in requestIpMap for every / request and can leak memory over time. Ensure you delete requestIpMap for this request before returning (or restructure so the mapping is only stored when needed).
| id: toolCallId, | ||
| name: part.name, | ||
| input: typeof part.input === 'string' | ||
| ? (JSON.parse(part.input || '{}') as Record<string, unknown>) | ||
| : ((part.input as Record<string, unknown>) || {}) | ||
| }); |
There was a problem hiding this comment.
processAnthropicMessages() parses LanguageModelToolCallPart.input with JSON.parse(...) when it’s a string. If part.input is not valid JSON, this will throw and fail the whole request. Wrap the parse in a try/catch (falling back to {} or the raw string) so malformed/non-JSON tool inputs don’t crash the Anthropic response path.
| const body = await this.readJsonBody(req) as AnthropicMessageRequest; | ||
| try { | ||
| const copilotModels = await vscode.lm.selectChatModels(); | ||
| const resolvedModel = this.resolveModel(body?.model); | ||
| const lmModel = copilotModels && copilotModels.length > 0 | ||
| ? (this.findModel(resolvedModel, copilotModels) || copilotModels[0]) | ||
| : null; |
There was a problem hiding this comment.
/v1/messages/count_tokens calls vscode.lm.selectChatModels() on every request, even though this class now has getCachedChatModels() specifically to avoid repeated model enumeration. Using the cached helper here would reduce overhead for clients like Claude Code that call this endpoint before every request.
| case 'getAuditSnapshot': | ||
| if (CopilotPanel.currentPanel) { | ||
| const val = data.value as any || {}; | ||
| const page = val.page || 1; | ||
| const pageSize = val.pageSize || 10; | ||
| const days = val.days || 30; |
There was a problem hiding this comment.
In the getAuditSnapshot handler, page, pageSize, and days are taken directly from data.value without type coercion. If the webview ever sends these as strings (common when values come from inputs), gateway.getDailyStats() / gateway.getAuditLogs() will receive the wrong types. Coerce/validate these to numbers (and clamp to sane ranges) before calling into the gateway.
suhaibbinyounis
left a comment
There was a problem hiding this comment.
The changes look solid. Verified locally. Tool calling, token counting fixes, and performance optimizations for the dashboard are properly implemented and well-engineered. Great work!
suhaibbinyounis
left a comment
There was a problem hiding this comment.
I initially approved this PR, but upon deeper review, I discovered a subtle memory leak related to how IP addresses are mapped.
In CopilotApiGateway.ts, the requestIpMap is populated at the beginning of the request:
this.requestIpMap.set(requestId, this.getClientIp(req));The intended cleanup currently happens inside this.logRequest() (this.requestIpMap.delete(requestId)). However, several fast-path endpoints (such as GET /, GET /health, GET /docs, GET /v1/models, and POST /v1/messages/count_tokens) return early via this.sendJson(...) and bypass this.logRequest() entirely. This orphans the requestId tracking strings, quietly exhausting the Extension Host's memory over time during automated health/token polling.
To fix this, you can decouple it from the logging mechanism entirely by tying the cleanup to the response completion event natively:
// set the IP at the start
this.requestIpMap.set(requestId, this.getClientIp(req));
// Guaranteed cleanup when response ends
res.on('close', () => {
this.requestIpMap.delete(requestId);
});Aside from this specific bug, the Anthropic structural payload fixes, model fallbacks, and dashboard optimizations are top-notch and look fantastic!
|
Thank you for pointing out this tricky bug, Now I have fixed it in the updated commit. |
suhaibbinyounis
left a comment
There was a problem hiding this comment.
Hey @gallexy-liu, this looks absolutely fantastic. The Anthropic API parsing implementation is incredibly robust, and the webview performance optimization (via statsSnapshot) is a massive win for the extension host's CPU footprint.
I've thoroughly checked the memory leak fix around requestIpMap. It's perfectly executed—tying the garbage collection to the underlying socket stream's res.once('close', ...) hook is the exact right Node.js architectural pattern to prevent early-return endpoints like / and /health from permanently stranding socket footprint references. To answer the question of how I originally caught the leak: I actually track and review deep codebase lifecycles using Google's Antigravity agent, which modeled the map's memory persistence and flagged sendJson bypassing the manual wrapper during fast-path routing!
One very minor note: JSON.parse(part.input) in the runWithConcurrency loop doesn't have a direct try/catch on it. If the local LLM hallucinates malformed JSON boundaries for the bounds of tool_use, it will throw an unhandled promise rejection internally which converts to a blunt 500 error to the client, instead of gracefully failing back to the LLM agent as a mapped tool failure. Not a blocker for this PR though, we can patch that JSON syntax edge case later down the line.
Terrific engineering all around, thanks for the contribution!
|
These changes are now live in version v2.11.0! 🚀 |
|
That's fantastic! |
"See changes: tool call handling, token counting, model fuzzy match, count_tokens endpoint, health handlers, image placeholder."